Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add double mixture privacy losses #277

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Add double mixture privacy losses #277

wants to merge 1 commit into from
+1,100 −316

Conversation

jan-schuchardt
Copy link

@jan-schuchardt jan-schuchardt commented Nov 13, 2024 •
edited
Loading

Hi,

this is a draft pull request related to issue #274.

I have implemented instances of AdditiveNoisePrivacyLoss whose privacy loss distribution is dominated by a pair of two mixture distributions. The class hierarchy looks as follows:

DoubleMixturePrivacyLoss
├── DoubleMixtureLaplacePrivacyLoss
├── DoubleMixtureGaussianPrivacyLoss
│   ├── MixtureGaussianPrivacyLoss

I have kept the existing MixtureGaussianPrivacyLoss as a special case of DoubleMixtureGaussianPrivacyLoss for backwards compatibility and because it has certain optimizations that are not implemented in its super classes.

Specifically, the DoubleMixtureXYZ classes are less optimized in the following sense:

  • No caching of constant terms in the privacy loss
  • No specialized heuristic for finding binary search bounds

These optimization cannot be trivially generalized to the super classes, because they rely on the (inverse) likelihood ratio decomposing into a sum of single-distribution ratios. This is not the case when we have two mixtures.

Before I create a final pull request, it would be great if we could discuss the following questions:

  • Can we live with the above optimization-related issues? (The implementation was still sufficiently fast for use with DP-SGD in my last project).
  • The DoubleMixtureXYZ classes are currently only being tested via tests for MixtureGaussianPrivacyLoss. Do we need separate tests for each class?

@jan-schuchardt jan-schuchardt mentioned this pull request Nov 13, 2024
Open
@jan-schuchardt jan-schuchardt force-pushed the double_mixture_pld branch 3 times, most recently from 9dfed70 to 11be431 Compare November 14, 2024 14:43
Add double mixture privacy losses
8c514a9 
DP Accounting changes:
* Add accounting for mechanisms dominated by a pair of two mixture distributions
* Add constructor method for discrete PLD of DoubleMixturePrivacyLosses
  to privacy_loss_distribution
@arung54
Copy link

arung54 commented Nov 20, 2024

Hi Jan, sorry for the delayed response. Wanted to follow up on this:

Can we live with the above optimization-related issues? (The implementation was still sufficiently fast for use with DP-SGD in my last project).

I think it is okay for now, and if someone requests we can work on optimizing later (either internally, or with your help).

The DoubleMixtureXYZ classes are currently only being tested via tests for MixtureGaussianPrivacyLoss. Do we need separate tests for each class?

It would be good to make sure any behavior specific to these classes are being tested. If you feel this is a lot of added work, we can potentially have you submit the current version and I can make a follow-up change that adds more robust tests.

In addition, there are some upcoming changes to the structure of privacy_loss_mechanism that might affect the PR (the amount of work needed to correct the PR for these changes should be pretty small). So it may be best to wait until those changes are out to make a final pull request :)

@jan-schuchardt
Copy link
Author

jan-schuchardt commented Nov 22, 2024 •
edited
Loading

So it may be best to wait until those changes are out to make a final pull request :)

All right, let's do that.

The existing tests do not look to complicated, so I can also take care of those.

@pritkamath
Copy link

pritkamath commented Nov 29, 2024 •
edited
Loading

Hi Jan.

The changes alluded to by Arun have been pushed now (see changes to privacy_loss_mechanism.py in this commit).

Basically, your PR made us realize that it would be more natural to introduce a common class of MonotonePrivacyLoss as a common abstraction of AdditiveNoisePrivacyLoss and MixtureGaussianPrivacyLoss.
Please sync your PR with those changes. Thank you and sorry about the extra trouble.

@jan-schuchardt
Copy link
Author

Ok, great. I'm on a vacation / conference travel for the next couple of weeks, but I'll look into doing the rebase after that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jan-schuchardt @arung54 @pritkamath